Skip to content

Conversation

mlautman
Copy link
Contributor

@mlautman mlautman commented Aug 12, 2019

Git seems to have messed up the diff. Any suggestions to avoid this?

@mlautman mlautman requested a review from davetcoleman August 12, 2019 08:25
@mlautman
Copy link
Contributor Author

@davetcoleman

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this more general!

* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionBlock(const geometry_msgs::Pose& block_pose, const std::string& name,
double block_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible for the CollisionObject msgs type to support color as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would have to change the msg

* \brief Create/Publish a MoveIt Collision block at the given pose
* \param pose - location of center of block
* \param name - semantic name of MoveIt collision object
* \param size - height=width=depth=size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to block size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


/**
* \brief Create a MoveIt Collision block at the given pose
* \brief Create/Publish a MoveIt Collision block at the given pose
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd rather you copy this doxygen for reach function. it seems these two functions due fairly different things, and the \brief should explain that difference better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/**
* \brief Create a MoveIt Collision block at the given pose
* \brief Create/Publish a MoveIt Collision block at the given pose
* \param pose - location of center of block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block_pose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* \param color to display the collision object with
* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionObject(const Eigen::Vector3d& point1, const Eigen::Vector3d& point2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be createCollisionCuboid right? Even better: createCollisionCuboidMsg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* \param color to display the collision object with
* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionObject(const Eigen::Isometry3d& pose, double width, double depth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments:
rename function to include Cuboid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

double depth, double height,
const std::string& name)
{
geometry_msgs::Pose pose_msg = tf2::toMsg(pose);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically in *_visual_tools, we use the built-in converter helper function rather than tf2, could you use that? e.g. convertPose()

same below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mlautman mlautman force-pushed the create-and-pub-separately branch 2 times, most recently from e626338 to 6c354a5 Compare August 18, 2019 09:45
@mlautman mlautman force-pushed the create-and-pub-separately branch from 6c354a5 to b274866 Compare August 18, 2019 09:46
@mlautman
Copy link
Contributor Author

@davetcoleman Done

@davetcoleman
Copy link
Member

Ive just retriggered travis, but i think you need to rebase on master with the travis fix you just made?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants